fix(baileys): always emit MESSAGES_UPSERT for media even when S3 upload is skipped or fails - #2684
Open
pastoriniMatheus wants to merge 3 commits into
Conversation
…ad is skipped or fails
In the messages.upsert handler, the S3 upload block returned early from the whole
handler in two cases (video upload disabled; getBase64FromMediaMessage returns
null), aborting before sendDataWebhook(Events.MESSAGES_UPSERT). The message is
persisted but the webhook carrying its content is never emitted.
This silently drops media messages whose upload is skipped or fails — notably
fromMe media sent from another device, where getBase64FromMediaMessage cannot
fetch the file (its mediaKey belongs to that device). Measured at ~94% media loss
for fromMe messages on a production deployment (S3 enabled).
Restructure the block so it skips only the upload, never the handler, so the
webhook is always delivered regardless of the storage outcome. The inline comment
("returning early from this block") shows the original intent was to skip the
upload only; another method in the same file already uses throw for the
equivalent case.
Contributor
Reviewer's GuideEnsures the WhatsApp Baileys messages.upsert handler always emits MESSAGES_UPSERT for media messages, even when S3 upload is disabled, skipped, or fails, by restructuring the S3 upload block to avoid early returns that previously aborted the handler. Sequence diagram for updated Baileys messages.upsert media handlingsequenceDiagram
participant BaileysStartupService
participant S3Service as s3Service
participant PrismaMedia as prismaRepository_media
participant PrismaMessage as prismaRepository_message
participant Webhook as sendDataWebhook
BaileysStartupService->>BaileysStartupService: messages.upsert(received)
alt isMedia && S3.ENABLE
alt isVideo && !S3.SAVE_VIDEO
BaileysStartupService->>BaileysStartupService: logger.warn('Video upload is disabled. Skipping video upload.')
note over BaileysStartupService: Skip upload only, continue handler
else nonVideo or S3.SAVE_VIDEO
BaileysStartupService->>BaileysStartupService: hasValidMediaContent(message)
alt !hasRealMedia
BaileysStartupService->>BaileysStartupService: logger.warn('Message detected as media but contains no valid media content')
else hasRealMedia
BaileysStartupService->>BaileysStartupService: getBase64FromMediaMessage(message, true)
alt media is null
BaileysStartupService->>BaileysStartupService: logger.verbose('No valid media to upload (messageContextInfo only), skipping MinIO')
note over BaileysStartupService: No upload, continue handler
else media available
BaileysStartupService->>S3Service: uploadFile(fullName, buffer, size, headers)
BaileysStartupService->>PrismaMedia: media.create(data)
BaileysStartupService->>S3Service: getObjectUrl(fullName)
BaileysStartupService->>PrismaMessage: message.update({ id: msg.id }, messageRaw)
end
end
end
else !isMedia or !S3.ENABLE
BaileysStartupService->>BaileysStartupService: proceed without S3 upload
end
BaileysStartupService->>Webhook: sendDataWebhook(Events.MESSAGES_UPSERT, messageRaw)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The S3 upload block is now quite deeply nested; consider extracting the media upload and persistence logic into a separate helper to simplify the messages.upsert handler’s control flow and improve readability.
- You are calling this.configService.get('S3') multiple times in close proximity; caching the S3 config locally within the handler would reduce repetition and make it clearer which configuration values are being used.
- The new explanatory comment above the try block is detailed but lengthy; trimming it down or moving the deeper context into a commit message would keep the code more focused while still documenting the behavior change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The S3 upload block is now quite deeply nested; consider extracting the media upload and persistence logic into a separate helper to simplify the messages.upsert handler’s control flow and improve readability.
- You are calling this.configService.get<S3>('S3') multiple times in close proximity; caching the S3 config locally within the handler would reduce repetition and make it clearer which configuration values are being used.
- The new explanatory comment above the try block is detailed but lengthy; trimming it down or moving the deeper context into a commit message would keep the code more focused while still documenting the behavior change.
## Individual Comments
### Comment 1
<location path="src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts" line_range="1603-1604" />
<code_context>
+ if (!media) {
+ this.logger.verbose('No valid media to upload (messageContextInfo only), skipping MinIO');
+ } else {
+ const { buffer, mediaType, fileName, size } = media;
+ const mimetype = mimeTypes.lookup(fileName).toString();
+ const fullName = join(
+ `${this.instance.id}`,
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against mimeTypes.lookup returning a falsy value before calling toString.
`mimeTypes.lookup(fileName)` can return `false`/`null` for unknown types, so `.toString()` may throw and break the upload flow. Please handle the falsy case (e.g. with a default like `'application/octet-stream'` or an explicit check) before converting to string.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1603
to
+1604
| const { buffer, mediaType, fileName, size } = media; | ||
| const mimetype = mimeTypes.lookup(fileName).toString(); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard against mimeTypes.lookup returning a falsy value before calling toString.
mimeTypes.lookup(fileName) can return false/null for unknown types, so .toString() may throw and break the upload flow. Please handle the falsy case (e.g. with a default like 'application/octet-stream' or an explicit check) before converting to string.
…3 upload is skipped or fails The `!media` early-return also exited the whole method, skipping sendDataWebhook(Events.SEND_MESSAGE) and `return messageRaw` — so POST /message/sendMedia responded empty. Mirror the messages.upsert fix: skip only the upload, never the method.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On the Baileys channel, media sent from the phone linked to the instance (
fromMe,source = android/ios) — audio, image, document, video — does not emit theMESSAGES_UPSERTwebhook whenS3_ENABLED=true. The message is persisted andMESSAGES_UPDATE(status) is emitted, but the event carrying the actual content never reaches the webhook consumer. Text from the same phone works; media sent through the API works. For any consumer (CRM, chatbot, archiver) this is silent, permanent loss — no error, no retry.Root cause — structural
src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts. The S3 upload block usesreturnto skip the upload, but the block sits beforesendDataWebhook(Events.MESSAGES_UPSERT, ...), so skipping the upload drops the webhook with it. Webhook delivery must never depend on the outcome of the S3 upload step — that is the defect.Concretely reachable on current
develop: the video-skip branchif (isVideo && !S3.SAVE_VIDEO) return.S3.SAVE_VIDEOdefaults to false (env.config.ts:process.env?.S3_SAVE_VIDEO === 'true', not set in.env.example), so it fires on every video in any S3-enabled install that didn't opt in — and thereturnis insidefor (const received of messages), so it aborts the rest of the batch, not just the video.Fix
Restructure the S3 block so it skips only the upload, never the handler — no
return/continueinside the block; the webhook is always emitted. Nothrowfor control flow; behavior unchanged when the upload succeeds. Two sites in this file share the pattern and are both fixed:messages.upsert(receive path) — droppedsendDataWebhook(Events.MESSAGES_UPSERT). This is the reported case.sendMessageWithTyping(API-send path) — the sameif (!media) returnskippedsendDataWebhook(Events.SEND_MESSAGE)andreturn messageRaw, soPOST /message/sendMediaresponded empty. Same restructure.Impact (measured on an affected production deployment)
fromMemedia delivery to the consumer went from ~7% to 100% (250/250 over 27 h), with no per-hour exceptions. A restart-vs-patch confound was ruled out: across three restarts within 11 minutes (the patch present only in the last), delivery was 0/5 on the restarts without the patch and 15/15 on the restart with it. Files remained intact in storage throughout.Behavior note
With
S3_SAVE_VIDEO=falseandWEBHOOK_BASE64=true, videos that previously vanished now flow through and the base64 block embeds the full video in the webhook payload. This is the previously-dropped media now being delivered — not a regression — but installs that disabled video upload to save bandwidth should be aware. Gate the base64 block by the sameSAVE_VIDEOflag if that's undesirable.Out of scope (called out on purpose)
fromMemedia rarely uploads to S3 at all (never getsmediaUrl) — pre-existing, not introduced here; separate issue.meta/whatsapp.business.service.ts) — separate PR, not mixed into this one.Testing
The repo has no test suite today (
npm testpoints at a non-existent./test/all.test.ts; the quality CI runseslint src+tsc --noEmit+tsuponly, which pass). Regression cases to lock in when a harness exists:messages.upsert: withS3.ENABLE=trueandS3.SAVE_VIDEO=false, avideoMessageanywhere in a batch must NOT stopsendDataWebhook(MESSAGES_UPSERT)from firing for the other messages in that batch.sendMessageWithTyping: same config,POST /message/sendMediafor a video must still return the message object and emitSEND_MESSAGE(not respond empty).